Skip to content

feat(auth): JWT authentication, RBAC administration and email delivery - #3

Merged
EdgarEldy merged 90 commits into
developfrom
feature/auth
Aug 26, 2026
Merged

EdgarEldy merged 90 commits into
developfrom
feature/auth

Conversation

@EdgarEldy

Copy link
Copy Markdown
Owner

Summary

This branch implements the full account lifecycle described in the README's feature/auth section: a custom JWT guard since the installed @adonisjs/auth ships no JWT support natively, Argon2 password hashing, rate limiting on the auth routes most likely to be abused, and register, activate, login, logout, refresh, forgot-password and reset-password all working end to end against a real Postgres and Mailhog instance. Registration and password reset now send real emails through SMTP rather than returning the raw token in the HTTP response, since a token a client can read is a token anyone reading that response can also use.

Two things extend beyond the README's original feature/auth scope, both added after discussion mid-branch and documented directly in the README itself. The first is a full ADMIN-only administration surface for users, roles and permissions, list, detail, update, delete, and assigning or revoking a role on a user or a permission on a role, since nothing in the original plan had any way to promote an account to ADMIN or manage the authorization model at all beyond direct database access. The second is the email delivery itself, replacing the original placeholder of echoing the activation and reset tokens in the API response.

A multi-angle code review after the bulk of the branch was implemented surfaced a substantial list of real findings, including a case-sensitive email column with no normalization anywhere in the auth flow, unbounded pagination limits on every admin list endpoint, a JWT that kept authenticating successfully after an ADMIN disabled or locked the account it belonged to, an unescaped HTML injection vector in the two mail templates, duplicate registration and role creation falling through to raw 500s instead of clean conflict responses, an Argon2 cost-amplification vector from unbounded password length, and a regression from develop where APP_KEY had lost its Secret redaction. All of these are fixed on this branch; the review's remaining lower-priority architectural findings, such as Bouncer sitting fully wired but unused alongside RoleMiddleware's own role check, are noted but left for a later branch to reconcile deliberately rather than restructured under this PR's time budget.

Task checklist

Every task in the README's feature/auth section is complete, including the extended administration and email delivery scope. JWT guard, Bouncer and Limiter are installed and configured; config/hash.ts uses Argon2 as the default hasher, driven by HASH_DRIVER. AuthMiddleware authenticates and rejects a blacklisted or revoked jti, and additionally rejects a disabled or locked account on every request, not just at login. RoleMiddleware preloads roles and checks them against the route's required roles. AuthService covers register, activate, login, logout, refresh, forgot password and reset password, with email delivery through Mailhog in every environment. Rate limiting is applied to register, login and forgot-password. The administration endpoints for users, roles and permissions are fully built with their own validators, services and controllers, all ADMIN only, with the same pagination and conflict-handling conventions as the rest of the API. Every listed unit and functional test, including the auth lifecycle, the middleware smoke test, the rate limiting test, and the full administration CRUD and authorization matrix, is written and passing.

Test plan

npm run typecheck, npm run lint and npm test all pass locally against a live PostgreSQL and Mailhog stack, 41 tests passing with zero failures, verified twice: once against an incrementally built local database, and once against a database reset to a single fresh migration batch to match what a clean CI run will see. Registration and password reset were also manually verified end to end against a running dev server, confirming the activation and reset emails actually land in Mailhog with the correct token, before any automated test was written against that flow.

feat(data-modeling): add Lucid models, migrations and seeders for the EER schema
This branch needs three packages that the scaffold never installed.
Bouncer and limiter are the authorization and rate limiting libraries
the README names explicitly for this branch, and neither ships with
the base @adonisjs/auth or core packages. A JWT signing library is
needed as well, since the installed @adonisjs/auth only ships session,
basic auth and access tokens guards, no JWT guard exists in the
package itself, so the custom guard this branch writes needs a real
implementation to sign and verify tokens against.

jose was chosen over jsonwebtoken for that last piece since it is pure
JavaScript with no native bindings, ships native ESM and TypeScript
types, and is the library the Node ecosystem has largely converged on
for JWT and JOSE operations going forward.
node ace configure @adonisjs/bouncer and node ace configure
@adonisjs/limiter both need their provider registered in adonisrc.ts
before the IoC container can resolve ctx.bouncer or the limiter
service anywhere else in the app, the same wiring every other package
in this file already goes through. The bouncer commands entry and the
indexPolicies hook are added for the same reason, so that ace can
discover policy classes and expose the make:policy generator.

Running the packages' own configure command rather than hand writing
this file avoids guessing at an API shape from memory, which is
exactly the kind of mistake that broke typecheck on the previous
branch.
ctx.bouncer only exists on the request context once
InitializeBouncerMiddleware has run, the same pattern
initialize_auth_middleware already follows for ctx.auth. It is placed
after silent_auth_middleware in the router stack since bouncer needs
to read the resolved user off ctx.auth when it builds the Bouncer
instance for the request, and that resolution has to happen first.
This middleware builds a Bouncer instance for each request, resolved
from ctx.auth.user, and attaches it to the context as ctx.bouncer so
downstream middleware, controllers and policies can call
bouncer.authorize or bouncer.allows without constructing their own
instance. It also augments the HttpContext type so ctx.bouncer is
typed against the abilities and policies actually registered in the
app, rather than an untyped any.

Generated by the package's own configure command rather than written
by hand, since this class needs to match the exact Bouncer constructor
signature of the installed version.
Abilities registered here can be referenced by name from a policy or
from Edge templates, and the file needs to exist before any policy
class can import from it. The placeholder editUser ability that ships
with the package's own scaffold is left in place for now rather than
deleted, since this branch does not yet define resource specific
abilities, those belong to the categories, products, customers and
orders branches once their own authorization rules are known.
LIMITER_STORE was already declared in the typed env schema back in
feature/core-architecture, set to database in every env file, but
nothing had actually configured the limiter package against that
setting until now. The database store persists rate limit counters in
the rate_limits table this branch also adds, which is the right
choice for this project since it already runs a shared PostgreSQL
instance in every environment and avoids introducing Redis as a new
piece of infrastructure just for throttling a handful of auth routes.

A memory store is kept alongside it since the limiter package ships it
by default and it costs nothing to keep as a documented alternative,
useful for a future test suite that wants throttling behavior without
touching the database.
This is the named throttle middleware the register, login and
forgot-password routes apply later in this branch, matching the
THROTTLE_AUTH_MAX and THROTTLE_AUTH_WINDOW values already present in
the env schema. The limit itself is wired directly to those env
values here rather than hardcoded, so tuning the throttle in a given
environment only ever requires an env change, never a code change.
The limiter package's database store needs a table to persist request
counters and their expiry against a key, generated directly by the
package's own configure command so its column names and types match
exactly what the store implementation expects to read and write.
…UTH_WINDOW

The previous commit on this file claimed the throttle was already
wired to the env driven auth limits, but it had actually only
committed the package's generated stub unchanged, a global limiter
hardcoded to ten requests per minute under the name global. This
commit corrects that: the exported throttle is renamed to authThrottle
and reads its request count and window straight from the typed env
service instead of a literal, matching what register, login and
forgot-password are supposed to apply later in this branch.

Caught while reviewing the previous commit's own message against the
file it actually produced, before any route wired the throttle in and
made the mismatch harder to notice.
HASH_DRIVER has been set to argon in every env file since
feature/core-architecture, and the README's tech stack table names
Argon2 explicitly as the password hashing algorithm for this project,
but the argon2 npm package itself, the native binding the hash
service's argon driver wraps, was never installed. Without it,
config/hash.ts could only ever resolve to the scrypt driver the
--kit=api scaffold shipped by default, regardless of what HASH_DRIVER
said.

The package ships prebuilt binaries for linux, darwin, freebsd and
win32 across common architectures, verified directly by hashing and
verifying a password with it before wiring it into config/hash.ts, so
no local compilation step is required in most environments including
this one.
The hash config inherited from the --kit=api scaffold hardcoded scrypt
as the only hasher and never read HASH_DRIVER at all, so setting that
variable in any env file had no effect. This branch introduces
password based login through AuthService, so the actual hasher the
project uses stops being a detail and starts being something the
README specifies directly, Argon2, the algorithm OWASP currently
recommends for password storage.

The default is now read from the typed env service instead of a
literal, and the argon entry uses the id variant with a 64 MiB memory
cost, three iterations and four way parallelism, values chosen to sit
well above argon2's own minimum recommended memory cost without
making login latency noticeable on ordinary hardware. Bcrypt is kept
configured as a documented fallback since HASH_DRIVER's schema in
start/env.ts already allows it, rather than deleting a working driver
the project might still want later.
The generated config read its default store straight from
env.get('LIMITER_STORE'), but that variable is typed as
'database' | 'redis' in start/env.ts, matching the README's
documented env reference table, while only a database store is
actually configured below. Running npm run typecheck against the
file as generated failed since 'redis' is not assignable to the
narrower type the configured stores record actually allows.

The default is hardcoded to 'database' instead, the same way
config/database.ts hardcodes its own connection rather than deriving
it from a variable that has no wired up alternative. Redis stays
undocumented as an option here until the project actually provisions
a Redis instance, at which point both the store and this default
would need to be added together.
The generated middleware included a guarded block sharing bouncer
helpers with Edge templates whenever ctx.view was present. This
project has no Edge or view provider registered anywhere, it is an
API only service per the README's own architecture, so ctx.view never
exists on the HttpContext type here and the guard could not narrow it
to anything usable, which failed npm run typecheck with ctx.view
being of type unknown.

Removing the branch rather than typing around it keeps the middleware
honest about what this project actually is. If a view layer is ever
added later, this is the file where that sharing call would come
back.
InitializeBouncerMiddleware imports #generated/policies, but that
file only starts existing once node ace codegen runs after the
bouncer package's indexPolicies hook is registered in adonisrc.ts.
Without it, npm run typecheck failed with a missing module error on
that import. The registry is currently empty since this branch does
not define any policy classes yet, categories, products, customers
and orders each own their own policy in their respective feature
branches.
Running the rate_limits migration against the live test database
regenerated this file automatically, the same way it did in
feature/data-modeling. It now includes the RateLimitSchema class
alongside the twelve tables from the core and auth schemas.
The custom JWT guard needs to live somewhere, and app/auth is where
that guard and any future auth specific helpers belong, distinct from
app/middleware and app/services which already have their own aliases.
Without this entry in the imports map, config/auth.ts could not
resolve #auth/jwt_guard through Node's subpath imports, the same
mechanism every other first party import in this project already
relies on instead of relative paths.
The installed @adonisjs/auth version ships session, basic auth and
access tokens guards, no JWT guard exists in the package itself, yet
the README's own code samples call ctx.auth.use('jwt') and read a
payload property off it, meaning the project expects a guard
registered under that name inside the normal auth guard system rather
than a bespoke authentication mechanism bolted on separately.

JwtGuard implements the package's own GuardContract interface, the
same contract SessionGuard and AccessTokensGuard satisfy, so the rest
of the app can treat it identically to a built in guard: authenticate,
check, getUserOrFail and authenticateAsClient for Japa's api client
test helpers. It is deliberately narrow in scope, verifying the
token's signature and expiry with jose and resolving the subject claim
to a real User is all it does. It does not check the jti blacklist or
the user's enabled and accountLocked flags, since the README assigns
that check to AuthMiddleware specifically, layered on top of
authenticate() rather than duplicated inside the guard.

JWT_EXPIRY is documented as either a bare number of seconds or a time
span like 7d, and jose's own parser only accepts the latter form for
strings, so a purely numeric value is converted to a number before
being handed to setExpirationTime. Signing is exposed as a standalone
signJwt function rather than a private guard method, since
AuthService's login and register flows need to produce a token
directly, not through a guard instance tied to an existing request.
feature/data-modeling left a session guard configured here purely as a
placeholder so the app would boot without the tokens guard the old
scaffold's User model could no longer support. This project
authenticates every request statelessly through a JWT in the
Authorization header, never through a cookie backed session, so the
session guard was never going to be more than scaffolding.

The default guard is now jwt, backed by the JwtGuard class this same
branch adds, which is what AuthMiddleware, RoleMiddleware and every
protected route from here on will actually authenticate against.
The README only names three of the six schemas this controller needs
(registerSchema, loginSchema, resetPasswordSchema) and leaves activate
and forgot-password unspecified. Both of those turned out to need their
own compiled schema anyway, an opaque token string for activate and an
email lookup for forgot-password, since the hard rule against inline
VineJS schemas in controllers applies to every action, not only the
three the README happened to name.

The password complexity rule (lowercase, uppercase, digit, on top of
VineJS's own minLength(8)) is a judgment call the README leaves open by
only asking for "a complexity regex" without specifying one. It is
shared between registerSchema and resetPasswordSchema so a password set
through either path meets the same bar, and deliberately left out of
loginSchema, which only checks whatever is already stored and should
fail with a credentials error rather than a validation error when the
input is merely short.

Field length caps on firstName, lastName and email mirror the users
table's VARCHAR limits from the README's Auth Model section, so a
too-long value is rejected as a 422 before it ever reaches a query that
would otherwise truncate or error against the database column.
JwtGuard proves a token is validly signed and not expired, nothing
more, by design (see app/auth/jwt_guard.ts). Revocation is a separate
concern: once logout writes a token's jti into blacklisted_tokens,
that token's signature is still perfectly valid until it naturally
expires, so something has to reject it anyway. This middleware is
that something, and it runs on every route that declares
middleware.auth(), which is also the only place JwtGuard is ever
invoked, so there is no path into the app that skips the check.

The jti is read from ctx.auth.use('jwt').payload after
authenticateUsing() succeeds, then looked up against
blacklisted_tokens with a single findBy. A hit returns 401 directly
from the middleware rather than throwing, matching the README's own
code sample for this file, and the response body is built out by hand
to match the ApiResponse envelope (including timestamp and path, which
the README's illustrative snippet omits for brevity but which every
other error path in this app, including the global exception handler,
includes) since this early return never reaches that handler.
The stub from feature/data-modeling passed every request through
unconditionally so route files could reference middleware.role()
before this branch existed. This replaces that stub with the real
check: authenticate, load the roles relation, and compare role names
against the options.roles argument passed at the route declaration
site, returning 403 in the ApiResponse shape when none match.

The explicit user.load('roles') call is not optional. The README's
troubleshooting section calls out, by name, that auth.authenticate()
never preloads relations and that skipping this load is the most
common reason RoleMiddleware ends up rejecting a user who does hold
the right role, since user.roles would silently be an empty array
rather than throwing. Calling auth.authenticate() again here, after
AuthMiddleware already called it earlier in the same request, is safe
and cheap: JwtGuard tracks authenticationAttempted internally, so a
second call in the same request returns the already-resolved user
instead of re-verifying the token from scratch.
This is the only place in the app that touches User, ActivationToken,
BlacklistedToken or PasswordResetToken for anything auth-related,
per the hard rule against direct model access in controllers.
register, activate, login, logout, forgotPassword, resetPassword and
me each map to one method, matching the README's task list exactly,
plus a refresh method the task list does not enumerate by name but
the Endpoints table requires ("Issue new JWT (if not blacklisted)").
Password hashing never happens here; User's @beforeSave hook does
that on every create and save, so register and resetPassword just
assign the plain password and let the model handle it.

Two decisions the README leaves open are worth calling out. First,
register and forgotPassword both return their raw token (activation
token and password reset token respectively) directly in the service
result rather than only persisting it, because there is no mail
provider configured on this branch and the README documents exactly
this placeholder for forgotPassword; register needed the same
treatment for the register -> activate -> login functional test flow
to be possible at all. Second, login and the credentials branch of
authentication raise the same E_INVALID_CREDENTIALS whether the email
does not exist or the password is wrong, so the response never
confirms which case occurred; forgotPassword does not extend that same
protection to email enumeration and uses findByOrFail instead, a
deliberate tradeoff toward explicit, testable behavior over that
particular hardening within this tutorial's scope.

Business exceptions (account not activated, account locked, token
already used, token expired, invalid reset token) are defined with
createError from @adonisjs/core/exceptions rather than thrown as raw
Error, each carrying its own status and code so the global exception
handler's generic "has a numeric status" branch formats them correctly
without any auth-specific handling added to that file.
Every action follows the same shape: validateUsing the matching
schema from auth_validator.ts, call the corresponding AuthService
method, wrap the result with respond(). None of the account lifecycle
logic (password verification, token validation, role assignment)
lives here, it all lives in AuthService, which is what the hard rule
against business logic in controllers actually means in practice for
this branch. The class is decorated with @Inject() since its
constructor takes AuthService as a dependency, resolved automatically
by the IoC container.

logout and refresh both read from ctx.auth.use('jwt') rather than
calling ctx.auth.authenticate() again, since middleware.auth() on
those routes has already authenticated the request and rejected a
blacklisted jti before the action runs; re-authenticating here would
just be redundant work against an already-cached guard instance.
logout additionally needs the raw bearer token string, which JwtGuard
does not expose after authentication (it only exposes the resolved
user and the decoded payload), so it is pulled directly off the
Authorization header here and passed to the service, which needs it
for the NOT NULL token column on blacklisted_tokens.
… stub

Every route under /api/v1/auth is declared here with its access rule
applied at declaration time, never added retroactively: register,
login and forgot-password carry .use(authThrottle) since they are the
three endpoints an attacker would script against (credential
stuffing, mass account creation, reset-token flooding), while
activate and reset-password are deliberately left unthrottled because
an unguessable token is already their real defense, and
logout/refresh/me are protected by middleware.auth() instead, which
already limits them to a valid, non-blacklisted bearer token. Method,
path and access all match the README's Endpoints table for this
branch exactly.

The two /api/v1/_stub routes exist only because the README's own test
checklist calls for a functional smoke test proving AuthMiddleware and
RoleMiddleware actually work end to end (401 with no token, 200 with a
valid USER token, 403 for USER and 200 for ADMIN behind
role('ADMIN')), and no real protected resource route exists yet on
this branch to hit; categories, products, customers and orders are all
later branches. Both routes are commented as temporary scaffolding and
should be deleted once feature/categories adds its first ADMIN-guarded
route, at which point that route's own functional test supersedes what
these stubs exist to prove.
These files are generated automatically from the controllers and
routes present in the app, and each carries its own header warning
against hand edits. Adding auth_controller.ts and the /api/v1/auth
routes (plus the two temporary middleware smoke-test stubs) changed
what the app actually exposes, so the checked-in snapshots were out of
date the moment those files were added.

Running node ace codegen regenerated the client-side route registry
(used by the Tuyau API client) and the server-side controllers and
routes type declarations to match. No source file changed here, only
the generated reflection of what already exists elsewhere in the tree.
…tests

There was no shared way for a functional test to obtain a JWT for one of the seeded ADMIN or USER accounts, which would have meant duplicating a login request and credential lookup in every spec file that needs an authenticated request. This helper reads TEST_ADMIN_EMAIL, TEST_ADMIN_PASSWORD, TEST_USER_EMAIL and TEST_USER_PASSWORD from the typed env service and logs in through the real POST /api/v1/auth/login endpoint, so callers exercise the same credential check and token issuance path a real client goes through instead of forging a token directly.

The file also exports ensureRole and ensureSeededUser. While writing these tests it became clear that tests/unit/database/migrations.spec.ts rolls every migration down and back up as part of its own coverage, which drops all seed data as a real side effect since that test does not wrap the migrator in a rollback-able transaction. When the unit and functional suites run together in one process, any auth test running after that migration test would otherwise find the roles table and the seeded accounts empty. These two helpers make each test group that needs that reference data create it for itself inside its own rolled-back transaction, so this branch's tests do not depend on seed data planted by an unrelated file earlier in the run.
…and logout

Before this commit AuthService had no test coverage at all, even though it owns the entire account lifecycle other branches will build on top of. There was nothing proving that register actually attaches the default USER role and produces a usable activation token rather than a dangling one, that activate really flips enabled and marks the token consumed so it cannot be replayed, or that login enforces its three failure paths, wrong password, locked account, inactive account, in the specific order the service documents. Getting the ordering wrong would matter in practice: a locked-but-inactive account should read back as not activated rather than locked, so the response never tells a caller more than it needs to know about why an account cannot sign in.

These tests hit the real test database inside a rolled back global transaction, following the same pattern used by the model specs from feature/data-modeling, instead of mocking the Lucid models AuthService depends on. The behavior under test only means something once the role pivot row, the activation token row and the blacklist row are actually read back from the database, so asserting against a mock would only restate the implementation rather than verify it. The login tests also decode the returned JWT to confirm the jti claim used later by the blacklist check is present and correctly tied to the user's id, and the logout test confirms the raw token and jti actually land in blacklisted_tokens with the right foreign key.
AuthMiddleware's reason for existing beyond calling auth.authenticate() is rejecting a request whose token has already been logged out, and that logic had no test pinned directly to it. The functional revoked-token scenario in tests/functional/auth.spec.ts does exercise this path end to end through a real HTTP request, but a failure there could just as easily come from the login or logout endpoints as from the middleware's own blacklist query, so it would not point at the actual cause. This test isolates AuthMiddleware's handle method against a minimal fake HttpContext exposing only the auth and response surface it actually reads, so a regression in the query against blacklisted_tokens fails here specifically instead of only showing up as a vague functional failure somewhere downstream.

The blacklist row itself is real and written to the test database inside a rolled back global transaction, since AuthMiddleware queries the BlacklistedToken model directly and there is nothing meaningful to mock at that layer. Two cases are covered, a jti that is present in blacklisted_tokens gets an unauthorized response and next is never called, and a jti that is not present lets the request continue through to next as normal, which is the behavior every authenticated route in the application depends on.
…nd rate limiting

This branch wires together a JWT guard, two middleware classes and an eight endpoint controller, but none of it had been exercised through a real HTTP request before this commit. Unit coverage on AuthService and AuthMiddleware proves each piece works in isolation, it does not prove register's activation token actually unlocks activate through the HTTP layer, that a token really becomes unusable on the very next request after logout rather than only in a direct service call, or that the role middleware genuinely tells an ADMIN token apart from a USER token using the seeded accounts rather than a hand built fixture. The full lifecycle test walks register, activate, login, a profile fetch, logout and a final rejected profile fetch as eight sequential requests against the running app, and the middleware smoke test hits the two temporary stub routes to confirm AuthMiddleware and RoleMiddleware actually gate access rather than merely compiling.

The rate limiting test needed particular care. Register, login and forgot-password all share a single named limiter keyed by client IP, so any of them running earlier in the same process, including the login calls the other two groups in this file make to obtain tokens, consumes from that same bucket. Rather than trying to guarantee this test runs before every other request that touches those three routes, it clears the rate_limits rows for that limiter's key prefix immediately before firing its own burst of requests, so it gets a clean window regardless of what already ran and in what order. It then sends exactly THROTTLE_AUTH_MAX successful requests followed by one more, and asserts only that last one comes back as 429, which is the actual contract callers depend on rather than an assumption about how many requests came before it in this run.
…as the role

A role still assigned to at least one user cannot be deleted, and must
fail with 409 rather than either silently succeeding, leaving that
user's role_user row pointing at a deleted role, or letting the
database's own FK constraint surface as an unhandled 500. Nothing in
this branch proved the guard actually runs before the delete, or that
a role really is removable once it is no longer assigned to anyone.

These tests hit the real test database inside a rolled back global
transaction, the same pattern as auth_service.spec.ts, since the rule
only means anything once a real pivot row exists to be checked
against.
…role has it

Mirrors RolesService's rule for roles still assigned to a user: a
permission still attached to at least one role cannot be deleted.
Without this guard, deleting a permission a role still grants would
either violate the role_permission FK constraint as an unhandled 500
or, worse, silently leave that role pointing at a permission row that
no longer exists.

These tests hit the real test database inside a rolled back global
transaction, the same pattern as roles_service.spec.ts, since the rule
only means anything once a real role_permission pivot row exists to be
checked against.
Two groups. The first proves the full CRUD lifecycle over real
requests, list, detail, update, assign a role, revoke it, delete, and
that every response matches the ApiResponse envelope, not just the
service layer in isolation. The second, and more important one, proves
something no test in this branch had touched yet: that assigning or
revoking a role through the admin API actually changes what a user's
own already-issued JWT is authorized to do on the very next request,
without any re-login.

Getting that wrong in either direction is dangerous. A stale check
that keeps trusting a revoked ADMIN role would let a demoted account
keep acting as an admin until their token expires on its own; one that
never picks up a newly granted role would force every promoted user to
log out and back in before the promotion takes effect. RoleMiddleware
calls user.load('roles') fresh on every request rather than caching
anything from the token itself, and this test is what actually proves
that design choice behaves the way its own code comments claim.
The unit spec for RolesService already proves its 409 guard throws the
right error at the service layer, but nothing proved the HTTP layer
actually turns it into a 409 response in the ApiResponse envelope
rather than falling through to a generic 500, or that the full create,
read, update, assign a permission, revoke it, delete cycle a real
ADMIN client would perform actually works end to end over real
requests rather than only through direct service calls.

The second test in this file walks the same 409 scenario through the
HTTP layer specifically: create a role, assign it to a user, attempt
to delete it and confirm 409, revoke the assignment, then confirm the
same delete now succeeds, proving the guard is not just correct in
isolation but wired all the way through the controller.
Mirrors roles.spec.ts's reasoning for the permission side: the unit
spec already proves PermissionsService.remove's 409 guard throws the
right error, but nothing proved the HTTP layer surfaces it correctly
or that the create, list, update, delete cycle works end to end.
Deletion is confirmed by re-listing rather than a detail route fetch,
since the README does not define a GET /api/v1/permissions/:id
endpoint and this test respects that rather than assuming one should
exist.
…ware

The README requires every user, role and permission administration
route to return 401 without a token and 403 with a non-ADMIN token,
but the CRUD tests in users.spec.ts, roles.spec.ts and
permissions.spec.ts only ever exercise a handful of routes with a
valid ADMIN token, they say nothing about the other sixteen. A route
added later that forgets middleware.role({ roles: ['ADMIN'] }), or
gets it applied in the wrong order relative to middleware.auth(),
would pass every other test in this branch while quietly granting a
regular USER account admin-level access.

This file exercises all seventeen administration routes twice, once
with no token and once with a seeded USER token, table-driven rather
than one test per route, so adding a new admin route later only means
adding one entry to the list instead of writing a new test. The ids in
each URL are placeholders since both checks happen in middleware
before any controller action reads the database, a route rejecting
with 401 or 403 never depends on whether that id actually exists.
…ec.ts

This file's group.setup() only ensured the ADMIN role existed (via
ensureSeededUser('admin')), but its second test registers a fresh
account through the public /api/v1/auth/register endpoint to have a
real user to assign the role under test to. AuthService.register looks
up the USER role by name and throws when it is missing, which the
exception handler turns into a 404, not the 201 this test expected.
Running the full suite exposed this immediately: users.spec.ts already
ensures USER in its own setup, this file never did.

Also casts response.body() at the two POST /api/v1/roles call sites.
That route resolves to a union of every action registered on the same
literal path (create and list), which TypeScript cannot narrow to the
specific shape a given response actually has, unlike the dynamic
:id routes elsewhere in this file that fall back to a loose type on
their own.
…llide

POST and GET both resolve to /api/v1/permissions and /api/v1/roles, so
response.body() is typed against the union of every action registered
on that literal path, and TypeScript cannot narrow .data to the shape
a specific call actually returns. npm run typecheck failed on every
property access following one of these calls. A small local body()
helper casts once at the call site instead of scattering the same
inline cast across ten different assertions.
…ests

npm run lint flagged formatting-only issues in these three files after
the roles.spec.ts and permissions.spec.ts fixes, caught during the
same verification pass. No assertions or test behavior changed.
…e same way

APP_KEY was declared as Env.schema.secret() on develop before this
branch, wrapping the value in a Secret object that redacts itself from
logs, console output and error traces. Feature/core-architecture had
already weakened it to a plain string() schema, losing that
protection for the key backing session and cookie encryption; this
branch's code review caught it while auditing everything touching
start/env.ts.

JWT_SECRET, new in this branch, signs and verifies every issued token
and deserves the identical protection: a debugging log statement or a
crash dump that happens to serialize env state should never print
either secret in plaintext. app/auth/jwt_guard.ts is updated in the
next commit to call .release() on the value, the only way to get the
raw string back out of a Secret.
JWT_SECRET is now Env.schema.secret(), so env.get() returns a
Secret<string> rather than a plain string. TextEncoder.encode has no
awareness of Secret the way config/encryption.ts's aes256gcm driver
does for APP_KEY, it needs the raw string, so this call site needs an
explicit .release() to unwrap it, matching the corresponding env
schema change in start/env.ts.
The mapping from a Lucid ModelPaginatorContract to this project's
PageResponse<T> envelope had been copy-pasted verbatim into
users_service.ts, roles_service.ts and permissions_service.ts, each
carrying a comment acknowledging the duplication and predicting the
categories, products, customers and orders services would repeat it a
fourth and fifth time. The code review flagged this from three
independent angles.

page_response.ts is where PageResponse<T> itself is defined and its
docstring already documented this exact mapping as the pattern every
service was expected to reimplement, so exporting the function from
here instead gives every current and future list endpoint one
implementation to fix instead of a growing set of hand-synced copies.
The three services are updated in the following commits to import it.
…nt reload

Three fixes to the same file, caught together during code review.
toPageResponse now imports from app/helpers/page_response.ts instead
of a locally duplicated copy. Email uniqueness checks in update() were
case sensitive, since none of the auth or user validators normalize
casing, meaning Test@Example.com and test@example.com could coexist
as two distinct rows against a column that is only unique per exact
string; the email is now lowercased before comparison and before
being persisted. assignRole and revokeRole each reloaded the roles
relation from the database a second time purely to refresh the
in-memory array after an attach or detach whose effect was already
fully known, an avoidable round trip since attach only adds the one
role already held in memory and detach only removes it; both now
mutate the loaded array in place instead.

user.roles is Lucid's opaque ManyToMany<typeof Role> at the type
level, array-like at runtime but not reassignable, so revokeRole
splices the detached role out by index rather than filtering into a
new array, which the type checker rejects as an assignment.
…undant reload

Same three fixes as the previous commit to users_service.ts, applied
to the role side. toPageResponse now imports from the shared helper.
create() and update() never checked roleName uniqueness before this,
even though roles.role_name carries a UNIQUE constraint at the
database level; a duplicate name fell through to a raw Postgres
constraint error with no .status property, which
app/exceptions/handler.ts's generic branch turns into an unhandled 500
instead of a clean 409, the exact gap UsersService.update already
guards against for email. assignPermission and revokePermission each
reloaded the permissions relation a second time after an attach or
detach whose effect was already known in memory; both now mutate the
loaded array in place, splicing by index on the revoke side since
role.permissions is Lucid's opaque, non-reassignable ManyToMany type.
Same pagination mapping duplication as users_service.ts and
roles_service.ts, now importing toPageResponse from
app/helpers/page_response.ts instead of a third locally duplicated
copy. Permissions has no uniqueness constraint in the schema (unlike
users.email and roles.role_name), so create() needed no equivalent
pre-check fix here.
Every list endpoint's index action coerced page and limit with a bare
Number(request.input(...)), duplicated identically across three
controllers, flagged from two independent code review angles. Beyond
the duplication, the coercion itself was unsafe: a non-numeric value
produced NaN, which Lucid's paginate() forwarded unchecked into the
SQL LIMIT/OFFSET clause, and limit had no upper bound at all, letting
a client request limit=999999999 and receive an entire table in one
response, exactly what the project's own no-unbounded-queries rule
exists to prevent.

page and limit are both optional and validated as positive numbers,
with limit additionally capped at 100. The three admin controllers are
updated in the following commits to validate through this schema
instead of coercing the raw query string themselves.
Replaces the bare Number(request.input('page'/'limit', ...)) coercion
with request.validateUsing(paginationSchema), closing the NaN and
unbounded-limit gaps that helper fixes for the whole admin surface.
Same fix as UsersController: request.validateUsing(paginationSchema)
replaces the unchecked Number() coercion on page and limit.
Same fix as UsersController and RolesController: request.validateUsing
(paginationSchema) replaces the unchecked Number() coercion on page
and limit.
The { success: false, message, timestamp, path } shape was hand-built
independently at five call sites: three branches of the global
exception handler, and once each in AuthMiddleware and RoleMiddleware.
respond() already exists in this file as the success-side counterpart;
fail() is the error-side equivalent, so both halves of the ApiResponse
contract live in one place instead of the success path being
centralized while the error path stays scattered. The five call sites
are updated in the following commits.
All three branches of the exception handler now call fail() from
app/helpers/api_response.ts instead of hand-building the same error
envelope three times. Also simplifies a redundant condition: the HTTP
exception branch cast error to { status?: number } and then checked
both the cast field's truthiness and typeof httpError.status ===
'number', two clauses expressing the same guard, since the cast type
already declares status as number | undefined. A single typeof check
covers both "present" and "numeric" in one clause.
…count

Uses fail() from app/helpers/api_response.ts instead of hand-building
the error envelope inline, matching the exception handler's own fix.

More importantly, this closes a real gap the code review caught: this
middleware only ever checked the jti blacklist, never enabled or
accountLocked, so a JWT issued before an ADMIN disabled or locked that
account via PUT /api/v1/users/:id would keep authenticating
successfully against every protected route until the token's own
natural expiry. AuthService only checks those flags at login and
refresh time, not on every request, so nothing else in the request
pipeline was catching this. The check runs immediately after
authenticateUsing() resolves the user, before the blacklist lookup,
since there is no reason to query blacklisted_tokens for a request
that is going to be rejected anyway.
Same dedup as the exception handler and AuthMiddleware: the 403
response now calls fail() from app/helpers/api_response.ts instead of
hand-building the same envelope shape a fifth time.
… mail errors

Three fixes to the account lifecycle, caught during code review.
register() never checked email uniqueness before User.create(), unlike
UsersService.update which already does for the same column; a
duplicate registration fell through to a raw Postgres unique
constraint violation with no .status property, which the exception
handler's generic branch turns into an unhandled 500 instead of a
clean 409. None of register, login or forgotPassword normalized email
casing either, so Ed@Example.com and ed@example.com could register as
two distinct accounts, and logging in with different casing than what
was registered would fail as invalid credentials even with the
correct password; every lookup and write now lowercases the email
first.

mail.send() in both register() and forgotPassword() was previously
unguarded. Both the user row and the activation or reset token are
already committed to the database by the time mail is sent, so an SMTP
failure (Mailhog unreachable, for example) must not turn an otherwise-
successful request into a 500 the client would retry straight into the
new E_EMAIL_TAKEN conflict, unable to activate an account they cannot
obtain a fresh token for. Both sends are now wrapped in try/catch and
logged rather than left to propagate.
…ector

None of registerSchema, loginSchema or resetPasswordSchema bounded
password length. config/hash.ts hashes with Argon2id, whose cost
scales with input size, and register and forgot-password are public,
unauthenticated endpoints; a client could submit a multi-megabyte
password field (config/bodyparser.ts allows large JSON bodies) and
force expensive hashing work per request, a cost-amplification vector
authThrottle's request-count limit alone does not prevent since it
caps how often a client can hit the endpoint, not how expensive each
hit can be.

maxLength(128) is added to all three password fields, including
login's, which intentionally still accepts any password shorter than
that cap without a complexity check, since login validates a
credential against whatever is already stored and rejecting a short
password before it reaches AuthService.login would turn a wrong
password into a confusing 422 instead of the expected 400
E_INVALID_CREDENTIALS.
registerSchema only bounds firstName and lastName by length, per the
README's Auth Model column widths, not by character set, so a value
like <img src=x onerror=alert(1)> passes validation. ActivationMail
and PasswordResetMail interpolate firstName directly into their .html()
bodies, so an unescaped value would render live in whatever HTML
capable client opens the mail, Mailhog in every environment this
project runs in, a stored injection vector reachable from
unauthenticated registration input. The following two commits apply
this helper at both call sites.
Closes the injection vector app/mails/escape_html.ts was added for.
The plain text body needs no change, there is no markup there to
inject into.
Same fix as ActivationMail, closing the same injection vector in the
other mail class that interpolates firstName into an HTML body.
… check

AuthMiddleware.handle now reads user.enabled and user.accountLocked
immediately after authenticateUsing() resolves the user, but this
file's fake HttpContext never actually returned a user from its
authenticateUsing mock, only undefined, which the middleware's new
check threw against. The mock now accepts and returns a real user
created in the test database.

A third test is added specifically for the new behavior: a request
carrying a valid, non-blacklisted jti for an account with
accountLocked set is still rejected, since neither of the two existing
tests exercised that branch, only the jti blacklist check the file's
own docstring says it exists to pin down.
@EdgarEldy
EdgarEldy merged commit e67ff86 into develop Aug 26, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant